Skip to content

[Oneshot Refactor] dataclass Arguments#1103

Merged
dsikka merged 39 commits intomainfrom
oneshot-refac-recipe_args
Feb 11, 2025
Merged

[Oneshot Refactor] dataclass Arguments#1103
dsikka merged 39 commits intomainfrom
oneshot-refac-recipe_args

Conversation

@horheynm
Copy link
Copy Markdown

@horheynm horheynm commented Jan 27, 2025

ORDER OF REVIEWS:

  1. [Oneshot Refactor] Rename get_shared_processor_src to get_processor_name_from_model #1108
  2. [Oneshot Refactor] dataclass Arguments #1103 <- current PR
  3. [Oneshot refactor] Refactor initialize_model_from_path #1109
  4. [Oneshot Refactor] Main refactor #1110

SUMMARY:
Refactor dataclass used for llm-compressor entrypoints (oneshot, train, apply) to decouple non-relevant attributes from the existing dataclass. Ex. recipe in training_args. Recipe is contained in a session, not in the trainer that training_args govern.

Dataclass refactor details are in
https://docs.google.com/document/d/1YbR1dTQmCzqhGk74m5msBzqoPHQgB6dVxDtf6cTmetc/edit?usp=sharing

Note:
#1110 takes care of using a new entrypoint that will prohibit the post_train / oneshot call to use training_args. Current entrypoint will need training_args for oneshot to function - this PR is just for refactoring the dataclass.

Before:
ModelArguments:


DataTrainingArguments:
class DataTrainingArguments(CustomDataTrainingArguments):

TrainingArguments:
class TrainingArguments(HFTrainingArgs):

After:
ModelArguments: https://github.com/vllm-project/llm-compressor/pull/1103/files#diff-58fd0f7ae4564317960ae0d4d4b2cdb97b9588c1915f062915e74ecf51b5502cR6
DatasetArguments: https://github.com/vllm-project/llm-compressor/pull/1103/files#diff-5e43f74ba5d8327b937adada3c7f30a7efb13f9a44cb3fdb5e1a2a12b8b8ea27R70
RecipeArguments: https://github.com/vllm-project/llm-compressor/pull/1103/files#diff-0ff9c048a4deb55e5459054bdc61a5d8c81da9c94588ec2355e6b2c2ec8675d1R6
TrainingArguments: https://github.com/vllm-project/llm-compressor/pull/1103/files#diff-249ee96763dd50956a7309f898eda4bcaa91c6af653474568fbda10b5a39c817R12

TEST PLAN:

  • Pass all existing tests
  • Search dataclass arguments using pygrep
  3 function pygrep() {
  2     local search_term="$1"
  1     shift
126     local search_dirs="${*:-src examples tests}"                           
  1     grep -rn --include="*.py" -E "$search_term" $search_dirs
  2 }

@github-actions
Copy link
Copy Markdown

👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review.

Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed.

@dsikka
Copy link
Copy Markdown
Collaborator

dsikka commented Jan 27, 2025

@horheynm Can you touchbase with Mark on the data arguments he shared during the meeting this morning?

@kylesayrs
Copy link
Copy Markdown
Collaborator

Is this ready for review? I'm a little confused what the comment "make changes in a different pr" means

@horheynm
Copy link
Copy Markdown
Author

@horheynm Can you touchbase with Mark on the data arguments he shared during the meeting this morning?

Yes

@horheynm
Copy link
Copy Markdown
Author

Is this ready for review? I'm a little confused what the comment "make changes in a different pr" means

It means it still needs to be broken down, it should only contain dataargs only.

@horheynm horheynm added the ready When a PR is ready for review label Jan 28, 2025
@kylesayrs kylesayrs marked this pull request as draft January 29, 2025 05:34
@kylesayrs kylesayrs added ready When a PR is ready for review and removed ready When a PR is ready for review labels Jan 29, 2025
@kylesayrs kylesayrs marked this pull request as ready for review January 29, 2025 05:35
@kylesayrs
Copy link
Copy Markdown
Collaborator

@horheynm Is this ready for review now?

@horheynm
Copy link
Copy Markdown
Author

@kylesayrs yes

@dsikka
Copy link
Copy Markdown
Collaborator

dsikka commented Feb 8, 2025

Please resolve conflicts

@dsikka
Copy link
Copy Markdown
Collaborator

dsikka commented Feb 8, 2025

https://docs.google.com/document/d/1YbR1dTQmCzqhGk74m5msBzqoPHQgB6dVxDtf6cTmetc/edit?usp=sharing

Does this cover everything?
Seems like we changed DataTrainingArguments to DatasetArguments but the doc just lists DataArguments?

Copy link
Copy Markdown
Collaborator

@dsikka dsikka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks like a good change but I would go one step further and move the args from under transformers altogether plus add docs for the restructured args.

Copy link
Copy Markdown
Collaborator

@brian-dellabetta brian-dellabetta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few comments for ya

@brian-dellabetta brian-dellabetta self-requested a review February 10, 2025 16:55
Copy link
Copy Markdown
Collaborator

@brian-dellabetta brian-dellabetta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but should re-request review from Dipika or Rahul before merging in

@horheynm horheynm requested a review from dsikka February 10, 2025 16:57
rahul-tuli
rahul-tuli previously approved these changes Feb 10, 2025
@horheynm horheynm dismissed stale reviews from rahul-tuli and brian-dellabetta via 7f49448 February 10, 2025 22:30
@dsikka dsikka enabled auto-merge (squash) February 11, 2025 01:12
@dsikka dsikka disabled auto-merge February 11, 2025 01:20
@dsikka dsikka merged commit e604f41 into main Feb 11, 2025
7 checks passed
@dsikka dsikka deleted the oneshot-refac-recipe_args branch February 11, 2025 01:20
dsikka pushed a commit that referenced this pull request Feb 11, 2025
ORDER OF REVIEWS:
1. #1108
2. #1103 
3. #1109 <- current
PR
4. #1110


SUMMARY:
Refactor `initialize_model_from_path` to decouple `training_args`
dependent logic and oneshot (non-training_args) logic.

TEST PLAN:
* Pass all tests
* search `initialize_model_from_path` using `grep`
dsikka pushed a commit that referenced this pull request Feb 24, 2025
ORDER OF REVIEWS:
1. #1108
2. #1103 
3. #1109
4. #1110 <- current
PR


SUMMARY:
* Create a class to decouple dependency to `main`. Class `Oneshot`
consists of pre-processing, carrying out oneshot logic and
post-processing
* Move the oneshot class and method under
`llmcompressor/entrypoints/oneshot.py`.
* Add ReadMe in `/llmcompressor/entrypoints` for info on oneshot
* Delete oneshot logic from `/finetune` directory, add deprecation
warning
* Remove apply used only for stagerunner oneshot pathway in session.py
and session_function.py
* Add oneshot only calibration dataloader logic
* Add a return variable of `model: PretrainedModel` for `def oneshot`.
* Make oneshot carryout logic independent of `TrainingArguments`
* remove `overwrite_output_dir` as oneshot input arg -> only used for
`TrainingArguments`
* Update README on `/finetune` path. Remove `oneshot` logic and `oneshot
with fsdp`
* Update `wrap_save_pretrained` logic to run only if not updated already
-> used for stage runner to avoid double wrapping



Entrypoints: 
```python3
from llmcompressor import oneshot
oneshot(**kwargs) # calls Oneshot
```

or 

```python3
from llmcompressor import Oneshot
oneshot = Oneshot(**kwargs)
oneshot() # preprocesss, carries out logic and post process
```


TEST PLAN:
Pass all tests and examples. 
Verified
https://github.com/vllm-project/llm-compressor/blob/main/examples/quantization_2of4_sparse_w4a16/llama7b_sparse_w4a16.py
works as expected.

FOLLOW UPS:
* Stage runner removal
* Update entrypoints folder with train, eval, predict, etc.

---------

Signed-off-by: George Ohashi <george@neuralmagic.com>
Signed-off-by: George <george@neuralmagic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready When a PR is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants